Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set InvariantGlobalization in api template #47066

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Set InvariantGlobalization in api template #47066

merged 2 commits into from
Mar 10, 2023

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Mar 7, 2023

API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix #47029

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 7, 2023
eerhardt added a commit to eerhardt/Benchmarks that referenced this pull request Mar 7, 2023
@eerhardt
Set InvariantGlobalization in BasicMinimalApi
b89d7b4 
The api template is updating, see dotnet/aspnetcore#47066. This syncs that change here.
@eerhardt eerhardt mentioned this pull request Mar 7, 2023
Merged
@mitchdenny
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

@captainsafia
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

I went ahead and took care of this in dotnet/msbuild#8543 since it was fairly straightforward to do.

@DamianEdwards
Copy link
Member

I'm onboard with this. Just want to represent @DamianEdwards opinion that we need to make sure that we've done the right thing in terms of MSBuild docs. We need that checklist in an issue so we can finalize this.

I went ahead and took care of this in dotnet/msbuild#8543 since it was fairly straightforward to do.

Right, we just need to follow up on VS property page support.

JamesNK
JamesNK previously requested changes Mar 8, 2023
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update other templates? webapi, grpc, worker?

Figure out how to test that culture is disabled. I'm not sure how to easily do this. The default template endpoints are returning invariant data already.

@eerhardt
Set InvariantGlobalization in api, webapi, grpc, and worker templates
f7073d4 
API server applications do not typically need culture aware behavior. A better default is to use invariant globalization mode for API apps.

This allows for the globalization code to be trimmed away, making the final executable size smaller. It also reduces startup and memory consumption because ICU isn't loaded.

This also allows the apps to run with consistent behavior on container images that don't include ICU, making the container image smaller as well.

Fix dotnet#47029
@eerhardt eerhardt requested a review from JamesNK March 9, 2023 23:38
@eerhardt eerhardt dismissed JamesNK’s stale review March 10, 2023 15:03

Responded to feedback

@eerhardt eerhardt merged commit 53e77ee into dotnet:main Mar 10, 2023
@eerhardt eerhardt deleted the UseInvariantGlobalization branch March 10, 2023 21:25
@ghost ghost added this to the 8.0-preview3 milestone Mar 10, 2023
eerhardt added a commit to aspnet/Benchmarks that referenced this pull request Mar 10, 2023
@eerhardt
Set InvariantGlobalization in BasicMinimalApi and BasicGrpc apps (#1803)
284d02f 
* Set InvariantGlobalization in BasicMinimalApi and BasicGrpc apps

The api template is updating, see dotnet/aspnetcore#47066. This syncs that change here.
@Varorbc
Copy link
Contributor

Varorbc commented May 11, 2023

@eerhardt If enabled by default,SqlClient will throw an exception

@ghost
Copy link

ghost commented May 11, 2023

Hi @Varorbc. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@Varorbc Varorbc mentioned this pull request May 11, 2023
Closed
@eerhardt
Copy link
Member Author

@eerhardt If enabled by default,SqlClient will throw an exception

The simple workaround in that case is to just remove this line from the .csproj then.

@ghost
Copy link

ghost commented May 11, 2023

Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@mitchdenny mitchdenny mentioned this pull request May 25, 2023
Open
@gfoidl gfoidl mentioned this pull request Nov 23, 2023
Closed
1 task
@vonzshik vonzshik mentioned this pull request Nov 23, 2023
Closed
@AlmightyLks
Copy link

AlmightyLks commented Nov 25, 2023 •
edited
Loading

FYI:
Breaks usage with EFC (Or SqlClient?) and thus makes ASP.NET Core 8 incompatible with EFC (/ SqlClient) by default

@ghost
Copy link

ghost commented Nov 25, 2023

Hi @AlmightyLks. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@roji
Copy link
Member

roji commented Nov 27, 2023

FYI this is purely a SqlClient-related issue that has nothing to do with EF per se.

@ghost
Copy link

ghost commented Nov 27, 2023

Hi @roji. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@eerhardt eerhardt mentioned this pull request Nov 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia approved these changes

@halter73 halter73 Awaiting requested review from halter73

@DamianEdwards DamianEdwards Awaiting requested review from DamianEdwards

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@JamesNK JamesNK Awaiting requested review from JamesNK

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
8.0-preview3
Development

Successfully merging this pull request may close these issues.

Consider setting InvariantGlobalization in api template
8 participants
@eerhardt @mitchdenny @captainsafia @DamianEdwards @Varorbc @AlmightyLks @roji @JamesNK